feat: optimize with branch prediction and cache-friendly structures #13
Conversation
WalkthroughRemoves parser buffering field, restructures parse flow to pass raw input, and reorganizes in-memory layouts (Value, Object, kv) for cache-friendliness. Introduces per-entry keyUnescaped handling, refactors whitespace/unescape logic, updates merge/set flows to unescape on-demand, and expands tests/benchmarks to exercise these paths. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
parser.go (3)
110-116: Avoid hard-coding byte sizes in comments (arch/Go-version dependent).
These “X bytes” claims can mislead across 32/64‑bit and different Go versions/GC. Consider dropping the exact counts or guard them with unsafe.Sizeof in a test/doc.
130-192: Switch-based dispatch is solid; consider removing duplicated NaN handling.
The dedicated 'n' branch handles null and also special-cases nan, but parseRawNumber already accepts ±inf/nan case-insensitively. You can drop the nan sub-branch under 'n' to reduce duplication without changing behavior.- case 'n': - // null literal - less common - if len(s) < len("null") || s[:len("null")] != "null" { - // Try parsing NaN - if len(s) >= 3 && strings.EqualFold(s[:3], "nan") { - v := arena.Allocate[Value](a) - v.t = TypeNumber - v.s = s[:3] - return v, s[3:], nil - } - return nil, s, fmt.Errorf("unexpected value found: %q", s) - } - return valueNull, s[len("null"):], nil + case 'n': + // null literal - less common + if len(s) < len("null") || s[:len("null")] != "null" { + return nil, s, fmt.Errorf("unexpected value found: %q", s) + } + return valueNull, s[len("null"):], nil
528-534: Same note as kv: byte-size comments are fragile.
Sizes vary by arch/alignment; prefer qualitative “hot/cold” notes without exact counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
parser.go(6 hunks)parser_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
parser_test.go (2)
handy.go (2)
Parse(125-128)ParseWithArena(130-133)parser.go (1)
Parser(38-40)
🔇 Additional comments (6)
parser.go (4)
86-90: LGTM: fast-path check stays branch-friendly.
93-108: Whitespace scan logic is correct and efficient.
The loop correctly skips only JSON‑whitespace and returns empty string if all-whitespace input.
313-326: LGTM: hot checks first improve branch predictability.
Order of '"'/'\' before control chars is sensible.
651-657: Mention Value field reordering in release notes. Repository scan found no unkeyed composite literals of Value or *Value.parser_test.go (2)
1295-1295: Bench: good practice to reuse buffer and check errors.
The out[:0] pattern avoids reallocs; error checks keep the benchmark honest.Also applies to: 1300-1301, 1304-1304
1312-1312: Arena bench: LGTM; buffer reuse + arena.Reset() per iter.
Matches the non‑arena benchmark pattern and keeps allocations bounded.Also applies to: 1317-1318, 1321-1321
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser.go (1)
599-621: Bug: Object.Get fails for keys containing escapes; compare logic needs adjustmentCurrent logic skips raw compare when the input contains '\', then unescapes stored keys but compares against the original (still-escaped) input, causing misses. This breaks lookups like Get("key\with\escapes") as tested.
Fix: always try a raw compare first; if not found and keys aren’t unescaped, unescape once and compare again.
func (o *Object) Get(key string) *Value { if o == nil { return nil } - if !o.keysUnescaped && strings.IndexByte(key, '\\') < 0 { - // Fast path - try searching for the key without object keys unescaping. - for _, kv := range o.kvs { - if kv.k == key { - return kv.v - } - } - } - - // Slow path - unescape object keys. - o.unescapeKeys(nil) - - for _, kv := range o.kvs { - if kv.k == key { - return kv.v - } - } + // First, try raw compare against stored keys (as parsed). + for _, kv := range o.kvs { + if kv.k == key { + return kv.v + } + } + // If not found and keys are not yet unescaped, unescape once and try again. + if !o.keysUnescaped { + o.unescapeKeys(nil) + for _, kv := range o.kvs { + if kv.k == key { + return kv.v + } + } + } return nil }
🧹 Nitpick comments (2)
parser_test.go (1)
361-364: Minor test message typos
- Line 362-364: want text should be "433" (not "443").
- Line 411-412: math.Inf(-11) should be math.Inf(-1) in the failure message.
- if string(sb) != "433" { - t.Fatalf("unexpected value; got %q; want %q", sb, "443") + if string(sb) != "433" { + t.Fatalf("unexpected value; got %q; want %q", sb, "433") } ... - if !math.IsInf(ninff, -1) { - t.Fatalf("unexpected inf_float value: %f. Expecting %f", ninff, math.Inf(-11)) + if !math.IsInf(ninff, -1) { + t.Fatalf("unexpected inf_float value: %f. Expecting %f", ninff, math.Inf(-1)) }Also applies to: 409-412
parser.go (1)
363-446: Optional: reduce allocations in unescapeStringBestEffortWhen emitting decoded runes, avoid []byte(string(r)) conversions. Use utf8.EncodeRune into a small stack buffer and append, which is allocation-free and cache-friendly.
- b = arena.SliceAppend(a, b, []byte(string(rune(x)))...) + var tmp [utf8.UTFMax]byte + n := utf8.EncodeRune(tmp[:], rune(x)) + b = arena.SliceAppend(a, b, tmp[:n]...) ... - r := utf16.DecodeRune(rune(x), rune(x1)) - b = arena.SliceAppend(a, b, []byte(string(r))...) + r := utf16.DecodeRune(rune(x), rune(x1)) + var tmp2 [utf8.UTFMax]byte + n2 := utf8.EncodeRune(tmp2[:], r) + b = arena.SliceAppend(a, b, tmp2[:n2]...)(Requires
import "unicode/utf8".)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
parser.go(7 hunks)parser_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
parser_test.go (3)
handy.go (5)
Parse(125-128)ParseWithArena(130-133)GetBool(95-103)ParseBytesWithArena(155-158)GetInt(59-67)fastfloat/parse.go (1)
Parse(359-508)parser.go (9)
Parser(38-39)NewParseError(24-30)ParseError(13-15)Type(701-701)TypeObject(708-708)TypeArray(711-711)MaxDepth(116-116)Object(527-531)Value(648-654)
parser.go (1)
handy.go (1)
ParseWithArena(130-133)
🔇 Additional comments (9)
parser_test.go (5)
1295-1305: Benchmarks: reusing marshal buffer and checking errors is solidGood changes: capturing parse result, checking errors, and reusing out[:0] prevents allocs and skew. LGTM.
Also applies to: 1312-1322
1326-1353: ParseError tests validate nil receiver and wrapping behaviorCovers nil/new errors and nil receiver method. Matches parser.go’s Error() semantics. LGTM.
1354-1387: Arena-based ParseWithArena testsCovers simple and nested cases; aligns with ParseWithArena signature. LGTM.
1390-1419: Arena ParseBytesWithArena testsExercising byte path for arrays/objects is useful; matches handy.go wrappers. LGTM.
1421-1452: skipWSSlow coverageGood coverage of allowed JSON whitespace and empty input. Matches implementation. LGTM.
parser.go (4)
47-52: Parse/ParseWithArena delegationSimple, clear delegation to unified parse. LGTM.
81-87: Whitespace skipping path is correct for JSON’s allowed whitespaceFast-path check and slow-path scanning over SP, LF, TAB, CR match RFC 8259. Tests cover this. LGTM.
Also applies to: 89-105
127-189: parseValue switch: ordering and error texts look goodBranching for string/object/array/literals with numbers in default is reasonable. Eager unescape for strings is consistent with MarshalTo. LGTM.
If strict JSON compliance is required, note plus-sign-prefixed numbers (“+123”) are accepted by parseRawNumber and may parse; confirm this is intended.
309-323: hasSpecialChars micro-optimizationsReordered checks for '"' and '\' before control range is sound. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser.go (1)
596-621: Critical: Object.Get slow path fails to unescape input key.The slow path unescapes stored keys but compares them against the original escaped input, so keys containing escape sequences never match.
Example:
- JSON:
{"key\\with\\escapes": "value"}- Stored key (raw):
"key\\with\\escapes"(with double backslashes,keyUnescaped=false)- Call
Get("key\\with\\escapes")(input with double backslashes)- Fast path skipped (input contains
\)- Slow path: unescapes stored key to
"key\with\escapes"(single backslashes), compares with input"key\\with\\escapes"(double backslashes) → no matchApply this diff to unescape the input key before comparison in the slow path:
// Slow path - unescape keys as needed and search + unescapedKey := unescapeStringBestEffort(nil, key) for _, kv := range o.kvs { if !kv.keyUnescaped { o.unescapeKey(nil, kv) } - if kv.k == key { + if kv.k == unescapedKey { return kv.v } }Note: This issue is already flagged in a previous review comment on parser_test.go lines 1610-1670 and remains unresolved.
🧹 Nitpick comments (2)
update_test.go (1)
123-465: Excellent test coverage for Object.Set.The comprehensive test suite covers nil handling, escape sequences, special characters, unicode, and various value types. The tests effectively validate the per-entry key handling introduced in this PR.
Note: Line 178 directly checks the internal
keyUnescapedflag. While this documents expected behavior, consider whether testing only public behavior (via Get operations) would be more maintainable if the internal implementation changes.update.go (1)
15-23: Fast path logic is correct but subtle.The fast path condition
!kv.keyUnescaped && kv.k == keyonly matches keys that haven't been unescaped yet (i.e., keys from parsed JSON). Keys added via Set havekeyUnescaped=trueand fall through to the slow path. This is correct but could benefit from a clarifying comment.Consider adding a comment explaining why the fast path only works for unescaped keys:
if strings.IndexByte(key, '\\') < 0 { - // Fast path - try searching for the key without unescaping + // Fast path - for keys without backslashes, try direct comparison with unescaped stored keys + // (keys from parsed JSON have keyUnescaped=false until first access) for i, kv := range o.kvs {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mergevalues.go(1 hunks)parser.go(10 hunks)parser_test.go(1 hunks)update.go(3 hunks)update_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
parser.go (1)
handy.go (1)
ParseWithArena(130-133)
update_test.go (2)
parser.go (9)
Object(528-531)Parser(38-39)Type(701-701)TypeNull(705-705)TypeString(714-714)TypeNumber(717-717)TypeTrue(720-720)TypeArray(711-711)TypeObject(708-708)handy.go (2)
MustParse(139-145)Parse(125-128)
parser_test.go (2)
handy.go (5)
Parse(125-128)ParseWithArena(130-133)GetBool(95-103)ParseBytesWithArena(155-158)GetInt(59-67)parser.go (9)
Parser(38-39)NewParseError(24-30)ParseError(13-15)Type(701-701)TypeObject(708-708)TypeArray(711-711)MaxDepth(117-117)Object(528-531)Value(648-654)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (12)
mergevalues.go (1)
38-43: LGTM: Per-entry key unescaping aligns with optimization goals.The change from bulk unescaping to per-entry conditional unescaping during object merges is correct and aligns with the broader per-entry key handling strategy across the codebase.
update_test.go (1)
467-577: LGTM: Edge case coverage is thorough.The edge case tests for repeated sets, set-after-get scenarios, and very long keys/values provide good coverage of potential issues.
update.go (1)
67-75: LGTM: Set method correctly handles per-entry unescaping.The Set method properly unescapes existing keys before comparison and marks new keys as already unescaped since they come from user input.
Also applies to: 81-81
parser_test.go (3)
1295-1305: LGTM: Benchmark improvements align with optimization goals.Adding the marshal step to benchmarks (reusing the
outbuffer) provides a more realistic measurement of the full parse-marshal cycle and exercises the memory optimization changes.Also applies to: 1308-1324
1326-1354: LGTM: New benchmark covers common usage pattern.The new
BenchmarkParseArenaAndGetbenchmark tests a realistic scenario of parsing, accessing multiple keys, and marshaling, which helps validate the performance impact of the per-entry key handling.
1357-2104: LGTM: Comprehensive edge case test coverage.The extensive new test suites provide thorough coverage of edge cases including:
- ParseError handling
- Arena-based parsing
- Whitespace skipping
- String escaping/unescaping edge cases
- Numeric boundary conditions
- Type conversion errors
parser.go (6)
46-52: LGTM: Clean refactoring unifies parse paths.Consolidating Parse and ParseWithArena to call a common
parsehelper eliminates code duplication and makes the arena parameter consistent.
82-105: LGTM: Whitespace skipping optimization is sound.The fast path early return and branch prediction optimizations in
skipWSSlowshould improve performance for common cases while maintaining correctness.
107-114: LGTM: Per-entry key state aligns with optimization goals.Adding the
keyUnescapedflag to thekvstruct enables lazy per-entry unescaping and reduces unnecessary work during parsing and lookups.
128-191: LGTM: Frequency-ordered dispatch should improve branch prediction.Reordering the switch cases by frequency (strings/objects/arrays first, literals last) should improve CPU branch prediction and reduce mispredictions in typical JSON workloads.
310-324: LGTM: Cache-friendly layout and documentation improvements.The struct reordering (hot fields first) and detailed comments about memory layout should improve cache locality. The MarshalTo optimization to check
keyUnescapedbefore escaping is correct.Also applies to: 526-531, 541-556, 646-654
577-584: LGTM: Dedicated unescapeKey method improves code clarity.Extracting the unescape logic into a dedicated method makes the per-entry unescaping pattern consistent across Get, Visit, Del, and Set operations.
2 participants
5% improvement on parse & marshal benchmark
Summary by CodeRabbit
Refactor
Bug Fixes
Tests